Add exhaustive divrem tests for ZZ #1919
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1919 +/- ##
==========================================
- Coverage 87.24% 87.21% -0.03%
==========================================
Files 97 97
Lines 35590 35588 -2
==========================================
- Hits 31049 31037 -12
- Misses 4541 4551 +10 ☔ View full report in Codecov by Sentry. |
joschmitt
reviewed
Oct 24, 2024
|
On Wed, Oct 23, 2024 at 09:06:59AM -0700, Lars Göttgens wrote:
Before I touch the implementations, I wanted to make sure to capture the current behavior.
Assumption: `Base.divrem(x,y) == (Base.div(x,y), Base.rem(x,y)` and `Nemo.divrem(x,y) == (Nemo.div(x,y), Base.mod(x,y))`
Good assumption - cannot be guarateend in general, unfortunately.
For rings with zero divisors (Z/nZ, ZK/Ideal, ...) the division is
non-unique and, in particular for ZK/Ideal, non-deterministic:
Hecke/src/NumFieldOrd/NfOrd/ResidueRing.jl
so, even if div and mod are implemented using divrem, since divrem is
random, the test will fail.
% is mapped to rem I think?
Thus the removal of the troubling code will kill
ZZ(3) % UInt(2)
This is total insanity - I wasn't aware of Nemo defining both
Nemo.divrem and Base.divrem
Outside of Nemo.jl I do not think anyone uses Nemo.divrem
Question for me would then rather be: can we remove it?
If we want the other pair, isn't there a "proper" Julia approach with a
"RoundingMode" to signal rem/ mod?
…
The few commented out tests are due to some weirdness with finite precision Ints together with unsiged Ints. All of the offending methods are in AA, and even in julia base are some discussions about what the return type of these should be.
One method I had to remove (there is a fallback via `UInt -> ZZRingElem`), because it did not satisfy the above assumption.
Does this look sensible?
You can view, comment on, or merge this pull request online at:
#1919
-- Commit Summary --
* Add exhaustive `divrem` tests for ZZ
* Remove troubling method
-- File Changes --
M src/flint/fmpz.jl (4)
M test/flint/fmpz-test.jl (87)
-- Patch Links --
https://github.com/Nemocas/Nemo.jl/pull/1919.patch
https://github.com/Nemocas/Nemo.jl/pull/1919.diff
--
Reply to this email directly or view it on GitHub:
#1919
You are receiving this because your review was requested.
Message ID: ***@***.***>
|
|
... Maybe rename Nemo.divrem into divmod? |
fieker
reviewed
Oct 25, 2024
| @@ -491,10 +491,6 @@ function rem(x::ZZRingElem, c::ZZRingElem) | |||
| return r | |||
| end | |||
|
|
|||
| function rem(a::ZZRingElem, b::UInt) | |||
| return ccall((:fmpz_fdiv_ui, libflint), UInt, (Ref{ZZRingElem}, UInt), a, b) | |||
There was a problem hiding this comment.
as this will remove ZZ(3) % UInt(2) I don't like it. Rather do an error if the input is illegal (ie. the remainder negative)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before I touch the implementations, I wanted to make sure to capture the current behavior.
Assumption:
Base.divrem(x,y) == (Base.div(x,y), Base.rem(x,y)andNemo.divrem(x,y) == (Nemo.div(x,y), Base.mod(x,y))The few commented out tests are due to some weirdness with finite precision Ints together with unsiged Ints. All of the offending methods are in AA, and even in julia base are some discussions about what the return type of these should be.
One method I had to remove (there is a fallback via
UInt -> ZZRingElem), because it did not satisfy the above assumption.Does this look sensible?